Skip to content

docs: explain LAMMPS exploration extra files - #393

Open
njzjz-bot wants to merge 1 commit into
deepmodeling:masterfrom
njzjz-bot:docs/issue-304-lammps-extra-files
Open

docs: explain LAMMPS exploration extra files#393
njzjz-bot wants to merge 1 commit into
deepmodeling:masterfrom
njzjz-bot:docs/issue-304-lammps-extra-files

Conversation

@njzjz-bot

Copy link
Copy Markdown

Summary

  • document input_extra_files for generated lmp-md task groups
  • show the DP-ZBL use_srtab table-file configuration
  • verify the normalized stage configuration copies the file content into each exploration task by basename

Tests

  • PYTHONPATH=tests python -m unittest -v tests.exploration.test_make_task_group_from_config.TestMakeLmpTaskGroupFromConfig
  • ruff format --check tests/exploration/test_make_task_group_from_config.py
  • isort --check-only tests/exploration/test_make_task_group_from_config.py
  • git diff --check

Closes #304

Coding agent: Codex
Codex version: codex-cli 0.149.0
Model: gpt-5.6-sol
Reasoning effort: xhigh

@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. documentation Improvements or additions to documentation exploration labels Aug 23, 2026
@coderabbitai

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 59 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9ed55d71-a5a7-4de8-88a1-0c1f1a9efa2c

📥 Commits

Reviewing files that changed from the base of the PR and between 6b01f29 and 23dee80.

📒 Files selected for processing (2)
  • docs/input.md
  • tests/exploration/test_make_task_group_from_config.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.45%. Comparing base (6b01f29) to head (23dee80).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
+ Coverage   84.43%   84.45%   +0.01%     
==========================================
  Files         104      104              
  Lines        6110     6110              
==========================================
+ Hits         5159     5160       +1     
+ Misses        951      950       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Document the input_extra_files path for DP-ZBL tables and verify that normalized lmp-md task groups copy each file into every task.

Coding-Agent: Codex
Codex-Version: codex-cli 0.149.1
Model: gpt-5.6-sol
Reasoning-Effort: xhigh
@njzjz-bot
njzjz-bot force-pushed the docs/issue-304-lammps-extra-files branch from b472b34 to 23dee80 Compare August 26, 2026 11:00

@wanghan-iapcm wanghan-iapcm left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finishing this bookkeeping - Closes #304 is correct. The feature landed in eb779cb (#306, 2025-08-29), six days after #304 was filed, but #306's body said "fix issue #305" (an unrelated resubmit crash), so #304 never auto-closed. #306 also claimed "The docs were updated too" while git show eb779cb -- docs/ is empty, so the remaining gap really is prose. I traced the mechanism end to end and it works: npt_task_group.py stores the basename and adds the file to every task, prep_lmp.py writes every key of files() into the task dir, and run_lmp.py symlinks everything under the task path into the LAMMPS work dir before the freeze step. The docs are accurate.

One thing to fix, with two surfaces.

The new test's fixture is degenerate in exactly the two dimensions the new paragraph makes claims about. The paragraph asserts (a) the file is copied "into every exploration task directory" and (b) it "is available there by its basename". The single new test pins neither, for two independent reasons, so one fixture change closes both. Details are in the two inline comments.

Evidence for (b), since it is the less obvious one - I reverted just the basename call in dpgen2/exploration/task/npt_task_group.py:

-        self.input_extra_files = [Path(ii).name for ii in input_extra_files]
+        self.input_extra_files = [str(ii) for ii in input_extra_files]

All 5 tests in tests/exploration/test_make_task_group_from_config.py still pass. For contrast, deleting the add_file loop that the test does cover fails it with KeyError: 'SiC_ZBL.txt', so the test is not vacuous - it just stops one step short of the property the docs advertise.

A suggested fixture that closes both at once: write the file into a subdirectory, pass sub/SiC_ZBL.txt (so basename stripping is observable), set "Ts": [100, 200] (so more than one task exists), and assert over every task in the group rather than tgroup[0].

Non-blocking notes, recorded so nobody has to rediscover them:

  • The branch no longer merges. git merge-tree --write-tree upstream/master 23dee80 conflicts in docs/input.md, because #372 added a #### PLUMED CV candidate filtering subsection into the same gap. On rebase the new paragraph should stay immediately after the n_sample paragraph, before that #### heading - appending it afterwards would make it read as part of the PLUMED subsection.
  • input_extra_files also exists on customized-lmp-template, where it means something different (files for the shell commands) and is genuinely not propagated: the inner lmp_tgroup.set_lmp(...) in _make_customized_task_group is called without it, so those files never reach the generated LAMMPS tasks. Your paragraph anchors to task_group[lmp-md] so it is not wrong, but that is a real separate bug worth its own issue.
  • The lmp-template half of #306 is still uncovered: deleting its add_file loop leaves all 62 tests under tests/exploration/ green. Out of scope here, just noting it did not come with #306 either.

Two things I checked and am explicitly not raising, so they do not get re-opened: assets/dp/SiC_ZBL.txt not existing is fine - the whole example block uses illustrative paths (/file/prefix, relpath/to/confs/*, template.lammps); and writing the fixture into the CWD matches how every test under tests/exploration/ has worked since 2022.

Comment on lines +64 to +78
def test_npt_copies_input_extra_files_to_each_task(self):
config = {
**self.config_npt,
"input_extra_files": [str(self.extra_file)],
}
tgroup = make_lmp_task_group_from_config(
self.numb_models, self.mass_map, config
)
tgroup.set_conf(["LAMMPS configuration"])
tgroup.make_task()

self.assertEqual(
tgroup[0].files()[self.extra_file.name],
"ZBL table content\n",
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This builds exactly one task, so the name ..._to_each_task and the docs claim "every exploration task directory" are both unverified here.

self.config_npt is {"type": "lmp-md", "Ts": [100]} with no press, so set_md leaves self.press = [None]; set_conf(["LAMMPS configuration"]) defaults n_sample to len(conf_list) == 1. make_task's itertools.product(confs, self.temps, self.press) is therefore 1x1x1. Since the assertion only reads tgroup[0], a regression that copied the extra file into only the first task of a multi-task group would leave this green.

"Ts": [100, 200] plus an assertion over every task in the group would pin it. I confirmed the real behaviour is correct - with two temperatures and two configurations all four tasks do carry the file - so this is about the test, not the code.


class TestMakeLmpTaskGroupFromConfig(unittest.TestCase):
def setUp(self):
self.extra_file = Path("SiC_ZBL.txt")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixture path has no directory component, which makes the basename behaviour invisible to the test.

Because this is the bare name SiC_ZBL.txt, Path(ii).name and str(ii) produce the identical string in npt_task_group.py, and replacing one with the other leaves all 5 tests passing. That is the specific property the new paragraph asserts ("available there by its basename") and the specific property the new example depends on - assets/dp/SiC_ZBL.txt is listed but referenced downstream as SiC_ZBL.txt.

Writing the fixture into a subdirectory and passing sub/SiC_ZBL.txt would make the stripping observable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation exploration size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing file for "use_srtab" in run_lmp

2 participants